[Internal] Replica Validation: Refactors Code to Address Follow Ups #3929
Conversation
…ddress Cache." This reverts commit ad5f6f0.
…er. Added cancellation token wire in.
| @@ -41,6 +41,7 @@ internal sealed class StoreClient : IStoreClient | |||
| bool useMultipleWriteLocations = false, | |||
| bool detectClientConnectivityIssues = false, | |||
| bool disableRetryWithRetryPolicy = false, | |||
| bool enableReplicaValidation = false, | |||
There was a problem hiding this comment.
Compute uses shared ServerStoreModel/Transport.
Thoughts on modeling options as request options?
| /// unhealthy one. The default value for this parameter is false. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para>This is optimal for workloads where latency spikes are critical during upgrades.</para> |
There was a problem hiding this comment.
Just mentioning upgrades in not right?
General verbatim would be ideal or for now exclude it.
There was a problem hiding this comment.
Ack. Taken a note on this. I will remove this from the original PR.
| /// <summary> | ||
| /// Gets or sets the prioritize healthy replicas flag. | ||
| /// Prioritizing healthy replicas helps the cosmos client to become more | ||
| /// resilient to connection timeouts, by choosing a healthy replica over an |
There was a problem hiding this comment.
Healthy or unhealthy is not defined and might be interpreted many ways.
| #else | ||
| internal | ||
| #endif | ||
| CosmosClientBuilder WithPrioritizeHealthyReplicas( |
There was a problem hiding this comment.
Did JAVA have it by-default? or public API?
There was a problem hiding this comment.
JAVA has it by the environment variables. I think it's enabled by default. Cc: @xinlian12
There was a problem hiding this comment.
yes it is controlled by environment variable, and it is enabled by default.
| @@ -47,6 +47,7 @@ internal class GatewayAddressCache : IAddressCache, IDisposable | |||
|
|
|||
| private readonly CosmosHttpClient httpClient; | |||
| private readonly bool isReplicaAddressValidationEnabled; | |||
| private static readonly TimeSpan WarmupCacheAndOpenConnectionTimeout = TimeSpan.FromMinutes(40); | |||
There was a problem hiding this comment.
What is 40 minutes? Why not leverage the incoming cancelaltion token
There was a problem hiding this comment.
We are leveraging the incoming cancellation token. The way it is done is, we add a Task.Delay(40mins, cancellationToken) and wait for the cancellation token to expire. The 40 mins here is just a place holder number to define the max wait time for opening up the connections. This can be polished later.
See this example for more detail around the approach.
There was a problem hiding this comment.
Incoming cancellation can be non-existent, so we need the LinkedTokenSource to guard against the process taking more than some expected time through a Task.Delay. The correct way to cooperate with a Task.Delay is what Deb is doing, through the linkedTokenSource
There was a problem hiding this comment.
If the incoming token cancels before, then it will cancel also the linked one, which cancels all Tasks. The linked token is also used to cancel the Task.Delay if the warmup finishes before, which is the expected scenario. Otherwise the Task.Delay keeps running in the back.
|
Draft PR. Closing as the original issue has been addressed. |
Pull Request Template
Description
Do not review this PR. This PR is meant to sneak peek the upcoming changes for the replica validation follow up works.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber